Feat: Origination Controller STIRFRY (Set Term Interest Rate Fixed Rate Yield) #134

PowVT · 2024-06-17T19:20:24Z

Add a new OriginationController contract which will be used to facilitate STIRFRY loans. Please read the OriginationControllerSTIRFRY.sol contract description for more details on how these loan are different from vanilla OriginationController loans.

A new test file has been added to achieve 100% test coverage of the new code.

Some slight refactoring of the OriginationController and OriginationControllerBase contracts was also necessary to achieve a clean implementation. Most notably, the _validateCounterparties() internal helper function was moved to the OriginationControllerBase contract so that its logic can be shared by the other OriginationController contracts.

kkennis

I feel a bit weird about reading "STIRFRY" everywhere, especially in all-caps form. On the protocol level, I would vote for a more bland (but also more descriptive) name for the financial primitive itself, like "interest swap" e.g. OriginationControllerInterestRateSwap or OriginationControllerIRS. I think the branch name itself is the right way to think about it on the protocol level, for example.

I don't think the intention was to ever brand this type of trade/financial primitive with "stirfry" - it's an existing primitive, with a name that already exists, called an interest rate swap. I think calling things stirfry "in-protocol" creates a layer of abstraction that has the danger of obstructing understanding of what the protocol actually does.

Stirfry might still be a good branding/go-to-market name for the "app" that allows people to take DeFi based interest rate swaps; you could imagine that the first line of docs could be "Stirfry is a DeFi protocol allowing counterparties to enter peer-to-peer interest rate swaps". But I'm not 100% sold on it as a go-to-market name either, so having it in the code locks us in and is potentially setting us up to have to go back and change it later.

I think the conceptual delineation is:

The Arcade protocol allows for peer-to-peer loans on fungible assets
To be doable on chain, an IRS contract needs to be trustless; mutual collateralization of assets is one way to enforce honest cooperation in the absence of trust and off-chain binding legal agreements
Putting 1 and 2 together above, the Arcade protocol is a system for collateralization (i.e. a lending protocol). Using a modified settlement flow, we can enforce mutual collateralization such that the loan notes reflect the rights to certain legs of the interest rate swap.
Given an on-chain primitive for an interest rate swap, we can build a product that gives users easy access to that swap - by product, this connotes not only a protocol, but a UI, a matching engine, and a revenue model. This product may be called Stirfry

Let me know if this makes sense, or if you disagree with the product framing and envision a tighter connection between the "Arcade Protocol" and the interest rate swap product currently codenamed Stirfry.

Also left a bunch of comments below, mostly around simplifying stirfryData to only include the vaulted currency and an implied unit-for-unit swap price, but also having those values be signed (because otherwise loans can be manipulated).

kkennis · 2024-06-17T22:17:52Z

contracts/interfaces/IOriginationControllerSTIRFRY.sol

+
+import "../libraries/LoanLibrary.sol";
+
+interface IOriginationControllerSTIRFRY is IOriginationControllerBase {


Will only leave this once in code, but see overall comment about naming

kkennis · 2024-06-17T22:24:30Z

contracts/origination/OriginationControllerSTIRFRY.sol

+ *     interest amount into the lenders vault. This vault is the vault that is being used for collateral in the loan.
+ * 3). There are no loan rollovers.
+ *
+ * In a STIRFRY loan scenario, a lender will mint a vault and deposit into the vault an ERC20 that accumulates some


Also want to challenge using the term "loan" here - the product itself is not a loan, it's a swap - the payoff curve for both counterparties looks different. I.e. the "borrower" (not really a borrower anymore) thinks of themselves as entering a "trade" as opposed to taking out a loan, and when they enter the trade they are not getting anything delivered to their wallet (not borrowing anything). For instance, there will be no "borrow" button on the product UI, it will be "fill order/trade". And there will be no "repay" button - it will be "settle/close position", which causes something that looks like repayment from LoanCore's perspective to happen, but that last part is just an implementation detail

kkennis · 2024-06-17T22:28:37Z

contracts/origination/OriginationControllerSTIRFRY.sol

+     *
+     * @return loanId                       The unique ID of the new loan.
+     */
+    function initializeStirfryLoan(


Same comment about loan, stirfry, etc....in my framing, it's more like initializeSwap

kkennis · 2024-06-17T22:29:12Z

contracts/origination/OriginationControllerSTIRFRY.sol

+        if(stirfryPairs[currencyHash] != true)
+            revert OCS_InvalidStirfryPair(loanTerms.payableCurrency, stirfryData.vaultedCurrency);
+
+        // verify the vaulted currency amounts
+        if(loanTerms.principal * stirfryData.payableToVaultedCurrencyRatio != stirfryData.lenderVaultedCurrencyAmount)
+            revert OCS_InvalidPrincipalAmounts(
+                loanTerms.principal,
+                stirfryData.payableToVaultedCurrencyRatio,
+                stirfryData.lenderVaultedCurrencyAmount
+            );


Historically we've always used braces around if clauses, and a space after the if keyword, not sure if this was an intentional style change or not

kkennis · 2024-06-17T22:32:29Z

contracts/origination/OriginationControllerSTIRFRY.sol

+                stirfryData.lenderVaultedCurrencyAmount
+            );
+
+        // verify that the vault holds the lenderVaultedCurrencyAmount


Should we verify that funds are held in the vault, or should we actually collect funds from the lender and send them into the vault at this time? Thinking about the scenario where the vault is created and funded but does not instantly get filled; if the vault is pre-funded, then the borrower earns that pre-swap yield, right?

See comments below as well; could we just collect loanTerms.principal * stirfryData.payableToVaultedCurrencyRatio?

Yes, agree the OC should bundle the assets together.

When implementing this I found that we need to mint the Asset Vault to the OC before collecting the ERC20 tokens because LoanCore does not implement the IERC721Receiver interface. Which just means there is an extra transfer of the vault from the OC to LC before calling loanCore.startLoan(). But I also think this is safer from a security standpoint since the vault factory uses safeMint() and there would be a callback on LoanCore if it was minted to LC.

kkennis · 2024-06-17T22:34:43Z

contracts/origination/OriginationControllerSTIRFRY.sol

+            / (Constants.BASIS_POINTS_DENOMINATOR * Constants.SECONDS_IN_YEAR);
+
+        // verify interest amounts
+        if(totalInterest * stirfryData.payableToVaultedCurrencyRatio != stirfryData.borrowerVaultedCurrencyAmount)


All these checks against the ratios seem to suggest that, if the ratio needs to match exactly, then there is no need for the StirfryData fields? See comment below on line 236

kkennis · 2024-06-17T22:34:51Z

contracts/origination/OriginationControllerSTIRFRY.sol

+        IERC20(stirfryData.vaultedCurrency).safeTransferFrom(
+            borrower,
+            vaultAddress,
+            stirfryData.borrowerVaultedCurrencyAmount


Could we just collect totalInterest * stirfryData.payableToVaultedCurrencyRatio?

kkennis · 2024-06-17T22:40:26Z

contracts/origination/OriginationControllerSTIRFRY.sol

+            address signingCounterparty = neededSide == Side.LEND ? lender : borrower;
+            address callingCounterparty = neededSide == Side.LEND ? borrower : lender;
+
+            (bytes32 sighash, address externalSigner) = _recoverSignature(loanTerms, sig, sigProperties, neededSide, signingCounterparty, itemPredicates);


Wouldn't vaultedCurrency need to be part of the signature? I think you need a new typehash for this; otherwise, let's say stirfryPairs allowed both sUSDe <> USDC and"uni v2 USDC<>USDT LP Token" <> USDC.

If I wanted to deliver fixed interest in USDC, then terms.payableCurrency would be USDC. Couldn't the counterparty swapping for fixed (the "lender" in terms of protocol designation) then call this function, but specify the other pair, the one against LP Tokens? It's not in my signature, so it seems like they could. Then all the sudden I'd be speculating on Uni V2 LP interest rates instead of sUSDe.

Made this change, good call. Rearranged the OriginationHelpers library contract a bit to add in the type hash (and encoding fn) and clean up a few things (moved isApprovedForContract() to OCBase). Signature recovery I put in the OCInterestRateSwap contract and kept it out of the OCBase contract.

kkennis · 2024-06-17T22:46:35Z

contracts/interfaces/IOriginationControllerSTIRFRY.sol

+        address lender,
+        Signature calldata sig,
+        SigProperties calldata sigProperties,
+        LoanLibrary.Predicate[] calldata itemPredicates


We don't want to use predicates at all here; it seems like that only confuses things? All the balances we need for the swap are already being checked by this contract.

kkennis · 2024-06-17T22:48:23Z

contracts/interfaces/IOriginationControllerSTIRFRY.sol

+        address vaultedCurrency;
+        uint256 lenderVaultedCurrencyAmount;
+        uint256 borrowerVaultedCurrencyAmount;
+        uint256 payableToVaultedCurrencyRatio;


Re: ratio here? Does this only imply a ratio in terms of the number of decimals each token uses, or does it also imply some "price ratio"? If it's decimals, should it be encoded in allowedPairs instead of being provided by a counterparty?

If it's provided by a counterparty, then it seems like the caller can put in whatever they want. If I were swapping variable-to-fixed, then I'd just make it 0 (it's also not validated anywhere, so I could). Then, line 200 would mean that stirfryData.borrowerVaultedCurrencyAmount would have to equal 0. That's great for me, I just opened a loan without having to actually vault anything.

Yup, the payableToVaultedCurrencyRatio is the decimals ratio. This ratio is currently being captured by the allowedPairs hash. See the hash setter function as well as the validation function.

Mouzayan · 2024-06-18T16:44:10Z

test/OriginationControllerSTIRFRY.ts

+            // Lender has 1,000,000 sUSDe they want to lock in a fixed rate of 15% APR on
+            await mint(sUSDe, lender, ethers.utils.parseEther("1000000"));
+
+            // Lender creates vault and deposits 1,000,000 sUSDe into it


In the comment: and deposits 900,000 vs. 1,000,000.

Mouzayan · 2024-06-18T17:12:33Z

contracts/rollover/CrossCurrencyRollover.sol

    ReentrancyGuard,
-    ERC721Holder {
+    ERC721Holder
+{


Can remove import "../libraries/FeeLookups.sol"; on line 20.

Mouzayan · 2024-06-18T17:22:30Z

contracts/errors/Lending.sol

+ * @notice The vaulted currency amount specified is not equivalent to what is actually vaulted.
+ *
+ * @param actualVaultedCollateralAmount         The actual balance of collateral ERC20 in the vault.
+ * @param proviededVaultedCurrencyAmount        The provided input for loan origination.


proviededVaultedCurrencyAmount typo in provided.

Mouzayan · 2024-06-18T17:22:38Z

contracts/errors/Lending.sol

+ */
+error OCS_InvalidVaultAmount(
+    uint256 actualVaultedCollateralAmount,
+    uint256 proviededVaultedCurrencyAmount


proviededVaultedCurrencyAmount typo in provided.

Mouzayan · 2024-06-18T17:38:42Z

contracts/origination/OriginationController.sol

-contract OriginationController is IOriginationController, OriginationControllerBase, FeeLookups, AccessControlEnumerable, ReentrancyGuard {
+contract OriginationController is
+    IOriginationController,
+    OriginationControllerBase,


Can remove import "../libraries/FeeLookups.sol"; from line 15.

Mouzayan · 2024-06-18T19:01:42Z

contracts/origination/OriginationControllerBase.sol

+     * @param sighash                   The hash of the signature payload (used for EIP-1271 check).
+     */
+    // solhint-disable-next-line code-complexity
+    function _validateCounterparties(


This function can stay in OriginationController and OriginationControllerSTIRFRY because those are the only 2 contracts using it. CrossCurrencyRollover which also inherits OriginationControllerBase, does not use _validateCounterparties. And other potential new contracts may not use it either.

The reason for putting it in the OCBase contract is so there is not duplicate code across two different OriginationControllers. Additionally, the OCBase contract is home to isSelfOrApproved() which gets called twice in _validateCounterparties() so it seemed to make more sense to have it located in the OCBase contract. Other OriginationControllers that inherit the OCBase that don't need _validateCounterparties() should not be affected since it is an internal function.

Mouzayan · 2024-06-18T19:12:32Z

contracts/origination/OriginationControllerSTIRFRY.sol

+ * This contract is similar to the vanilla OriginationController, with a few key changes:
+ * 1). The collateral is collected from the lender instead of the borrower.
+ * 2). The borrower does not receive any principal at the start of the loan. Instead, the borrower deposits the
+ *     interest amount into the lenders vault. This vault is the vault that is being used for collateral in the loan.


lender's vault vs. lenders.

Mouzayan · 2024-06-18T19:18:13Z

contracts/origination/OriginationControllerSTIRFRY.sol

+ * the same ERC20 into the vault that the lender deposited. The amount of ERC20 that the borrower deposits is
+ * equivalent to the fixed rate yield that the lender is signaling in their signed loan terms. When a borrower
+ * deposits the fixed rate yield amount into the lender's vault, a loan will be originated. Upon loan origination,
+ * the borrower will not receive any principal. Instead, they are reserving the right to repay the loan at any time


May be worth adding a test where the borrower repays the loan before the loan duration ends. All existing test that show successful repayment are repaid after the loan duration is passed.

Repayment flows are all tested in RepaymentController.ts.

I tested one repayment flow in the STIRFRY tests, just to show what the e2e flow looks like and how the amount of tokens the borrower needs to front in to receive the vaulted collateral. This can be extended to all repayment scenarios prior to the end of the loan. The borrower just needs to repay the prorata amount.

Mouzayan · 2024-06-18T19:53:55Z

contracts/origination/OriginationControllerSTIRFRY.sol

+
+    /**
+     * @notice Perform loan initialization. Pull fixed interest amount from the borrower and add to vault.
+     *         Take custody of collateral form the lender. Tell LoanCore to create and start a loan.


from the lender vs. form

Mouzayan · 2024-06-18T20:12:58Z

contracts/origination/OriginationControllerSTIRFRY.sol

+     * @param isAllowed                  Whether the pair is allowed.
+     */
+    function setPair(address currency1, address currency2, uint256 ratio, bool isAllowed) external onlyOwner {
+        bytes32 key = keccak256(abi.encodePacked(currency1, currency2, ratio));


Although the likelihood of hash collisions is very low in this context, we should be aware that generating keys through hashing could theoretically lead to issues.
But given that we are only dealing with a limited number of whitelisted pairs, our risk is very low.

Yeah, noted on the hash collisions, I would say if there were hundreds of things being whitelisted I would be a little more concerned. But due to the low number of combinations I think will be supported, I had the same reasoning as you that the likelihood is extremely low.

Mouzayan · 2024-06-18T21:16:08Z

contracts/origination/OriginationControllerSTIRFRY.sol

+        address vaultAddress
+    ) internal view {
+        // verify the vaulted currency is allowed to be paired with the loan terms payable currency
+        bytes32 currencyHash = keccak256(


Maybe this var can just be called key instead of currencyHash because it is not really a hash of any currency.

Mouzayan · 2024-06-18T21:21:10Z

test/OriginationControllerSTIRFRY.ts

+    maxUses: 1,
+};
+
+describe("OriginationControllerSTIRFRY", () => {


It's worth adding a test where the payableToVaultedCurrencyRatio is a fraction or more complex than 1:1.

Since payableToVaultedCurrencyRatio is a uint, it cant be a fraction. The example in the tests uses a 1e12:1 ratio for sUSDe:USDC.

payableToVaultedCurrencyRatio is trusted input from the contract owner. There are notes about this in the natspec for setPair() detailing how to calculate this ratio and what it is used for.

…ents

PowVT added 11 commits June 14, 2024 17:22

initial renderings

9346b40

alternatives

b159706

happy path tests

cd54e09

add invariant protection, pull fixed rate from borrower

f9c6a27

stirfry integrate w OC base

ea1a329

rm FeeLookups from all OC contracts

2161a9a

move _vaildateCounterparties to oc base

5edc659

stirfry tests, custom errors

113cce8

add test for currencies w/ incompatible decimals

dba7599

fix var naming, update natspec

3b6b8bd

rm unused errors

34b374e

PowVT requested review from Mouzayan and kkennis June 17, 2024 19:20

kkennis reviewed Jun 17, 2024

View reviewed changes

Mouzayan reviewed Jun 18, 2024

View reviewed changes

PowVT added 4 commits June 28, 2024 09:13

change naming convention to interest rate swap

f528119

update SwapData struct, create vault at time of origination, add comm…

c35874b

…ents

add vaulted currency to interest rate swap sigs

946d614

add interest rate swap contract desc natspec

3f8d88f


		import "../libraries/LoanLibrary.sol";

		interface IOriginationControllerSTIRFRY is IOriginationControllerBase {

Feat: Origination Controller STIRFRY (Set Term Interest Rate Fixed Rate Yield) #134

Are you sure you want to change the base?

Feat: Origination Controller STIRFRY (Set Term Interest Rate Fixed Rate Yield) #134

Uh oh!

Conversation

PowVT commented Jun 17, 2024

Uh oh!

kkennis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mouzayan Jun 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Mouzayan Jun 18, 2024 •

edited

Loading